-
Notifications
You must be signed in to change notification settings - Fork 118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
POST request to an API #111
POST request to an API #111
Conversation
@GouravSardana @pranavbaburaj @NamamiShanker review pls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for contributing to dynamic cli. I have added some review comments. Please let me know your thoughts. 🙂
Share a GIF or some video to show on various input. So that other can also see how they can use it ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make these small changes and really thankful for your contribution. Great work 🥇
src/arguments/api_test.py
Outdated
cls.read_data_from_file() | ||
@classmethod | ||
def fetch_payload_data(cls): | ||
store = int(input("Please choose the below options? (1/2/3)")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a option here ?
Do you want to send payload Y/N
Then we can display option like send a payload from terminal or file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then there would be multiple conditions
if yes :
enter 1 to send from terminal
enter 2 to read from terminal
any other number #then again we have to repeat from starting line, if you want to send payload
elif no
return
else
recur the function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also send bearer token in this payload ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah if the user sends it
src/arguments/api_test.py
Outdated
@@ -84,6 +116,20 @@ def get_request(cls): | |||
print(cls.invalid_schema_message) | |||
except Exception as exception_obj: | |||
print(exception_obj) | |||
#Make a POST request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add 2 space above. Follow pep8 formatting please
src/arguments/api_test.py
Outdated
request_data = cls.fetch_input_url() | ||
data = cls.fetch_payload_data() | ||
try: | ||
response= requests.post(url = request_data["request_url"], headers= request_data["request_headers"], data = data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a space before = (equals)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
everything can be done, using black formatter I guess, shall we try adding it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, you can use the black formatter locally on your system before pushing it 😃
It's not working on my OS |
How do you test it then ? |
I tried testing in gitpod, running into errors, new to Linux environment |
No issues. I'll test it before merge @harshakhmk . You can make your changes which are requested |
@GouravSardana Thank you for understanding |
I have made the suggested changes |
Any other changes? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any other changes?
I think, no. @GouravSardana, please let us know what you think!
Some Suggestions from my end -
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I request the changes above, Please work on it
For testing any End Point - You can use this - https://reqres.in/ |
Oh, my bad. Strings don't have an attribute called if len(filename.strip()) == 0:
print("filename empty, so default file (response_data.json) is used ") I had the wrong method in the example. I am sorry 😐 |
The First 3 issues were resolved, for the time being can we add issue 4 as a new feature in a separate PR @GouravSardana |
Did you push your changes after doing these first 3 issues |
@GouravSardana yes, I have updated the code |
I'll try it locally once. If everything went well. I'll merge it. Please create an issue for the last screenshot requirement which i have shared above |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's good. 🙂
Kudos, SonarCloud Quality Gate passed!
|
Everything looks good. Thanks for the contribution @harshakhmk |
Related Issue
Closes: #https://github.com/IndianOpenSourceFoundation/dynamic-cli/issues/76
Describe the changes you've made
Checklist: